RFC: JavaScriptCore-safe bundle URLs #646

robhogan · 2023-05-02T19:51:39Z

Proposes a solution to the issues (facebook/react-native#36794, expo/expo#22008) caused by the change to JavaScriptCore that strips information from stack trace source URLs.

View the rendered RFC

…RL-encoding"

tido64 · 2023-05-09T14:38:46Z

proposals/0642-JavaScriptCore-safe-bundle-URLs.md

+ - Maintenance/complexity of supporting an additional URL format.
+ - Forking of bundle URL generation in React Native by platform/engine, assuming we use this new format on as narrow a range of target engines as possible.
+
+## Alternatives


Not entirely familiar with all the bits involved, but I have a couple of questions:

What prevents Safari from also stripping out ;& for security reasons?

Have you considered embedding a comment similar to //# sourceMappingURL= for source maps?

What prevents Safari from also stripping out ;& for security reasons?

Nothing in theory, but my understanding is that query strings (which of course aren't fundamentally less secure than path components) are commonly used to append tracking or other sensitive information in a web context during redirection. From what I can gather (though the security bugzilla is private), the JSC change was to plug quite a specific hole where stack consumers could end up being sent the sensitive data added by a separate redirection service. Those services would have to motivation to make that private data leakable, and couldn't append ;& in the general case anyway because it will usually break routing.

I've also been in touch with Apple folks about building an opt-out for controlled, non-browser use cases like RN - they're open to that because the disruption to our use case was completely unintentional, so I don't think we're getting into an arms race here - just a workaround for a temporary situation.

Have you considered embedding a comment similar to //# sourceMappingURL= for source maps?

Metro already adds these, but JavaScriptCore doesn't use them when populating error stacks. The stacks coming from the engine only use the sourceURL provided explicitly to the JSEvaluateScript call (docs).

robhogan · 2023-05-15T15:54:34Z

proposals/0642-JavaScriptCore-safe-bundle-URLs.md

+
+Use a character sequence from the "reserved" set specified in RFC 3986, namely `;&`, as an alternative delimiter in place of `?`, and percent-encode any `?` characters appearing within the subsequent query string, so that the result is a valid [`path`](https://www.rfc-editor.org/rfc/rfc3986#section-3.3).
+
+If the client sends a URL in this format, Metro should respond as if `?` was seen in place of the alternative delimiter, with the following exceptions:


@motiz88 I think we need to specify what "in this format" means here. For example, what format is this?

`/path/with-;&-unencoded/index.bundle?platform=ios`

If we interpret it as a "URL of mixed format" as suggested below, then we'll break the specific edge case we claim to support, because clients are not presently required to percent-encode this sequence within a bundle request path (and in fact, don't). In practice, we could ensure that React Native escapes that sequence in future, but it'd still technically be a semver-breaking change to Metro, albeit in a narrow edge case.

It might be more practical to consider an input to be "in this format" if it contains both a ;& within the path, and no query component.

Another possibility is to use a character sequence that can't appear in (normalised) POSIX paths - there aren't many, but maybe //?

robhogan · 2023-05-18T14:49:44Z

I've just made a substantial update to the RFC in a961392, in summary:

The previous choice of delimiter ;& did not allow us to disambiguate an encoded URL from an existing URL with ;& coincidentally appearing in a file path, without assuming a client would percent-encode more of the path than it was required to. In particular, pre-existing clients would not be supported, which would make this change semver breaking. The choice of //& is explained in the RFC.
I'm now proposing that we adopt //& as the delimiter in all emitted "source URLs", and not requiring clients to update their request URLs in order to take advantage of the fix. Bundle request URLs are user-overridable in React Native, so our ability to control their formatting in a non-breaking patch is limited. By instead conceptually separating request URLs from source URLs, we can return a source URL along with the source from bundlers, and use that downstream of the http fetch.

Summary: This is the iOS side of the fix for facebook#36794. That issue aside for the moment, the high-level idea here is to conceptually separate the bundle *request URL*, which represents a request for the *latest* bundle, from the *source URL* passed to JS engines, which should represent the code actually being executed. In future, we'd like to use this to refer to a point-in-time snapshot of the bundle, so that stack traces more often refer to the code that was actually run, even if it's since been updated on disk (actually implementing this isn't planned at the moment, but it helps describe the distinction). Short term, this separation gives us a way to address the issue with JSC on iOS 16.4 by allowing Metro to provide the client with a [JSC-safe URL](react-native-community/discussions-and-proposals#646) to pass to the JS engine, even where the request URL isn't JSC-safe. We'll deliver that URL to the client on HTTP bundle requests via the [`Content-Location`](https://www.rfc-editor.org/rfc/rfc9110#name-content-location) header, which is a published standard for communicating a location for the content provided in a successful response (typically used to provide a direct URL to an asset after content negotiation, but I think it fits here too). For the long-term goal we should follow up with the same functionality on Android and out-of-tree platforms, but it's non-essential for anything other than iOS 16.4 at the moment. For the issue fix to work end-to-end we'll also need to update Metro, but the two pieces are decoupled and non-breaking so it doesn't matter which lands first. Changelog: [iOS][Fixed] Use `Content-Location` header in bundle response as JS source URL Differential Revision: D45950661 fbshipit-source-id: 0c4f74c5cac4e2e0bf0e251614f55bd58685619d

Summary: The remaining Metro part of the implementation of react-native-community/discussions-and-proposals#646, to fix (along with an RN change): - facebook/react-native#36794 and - expo/expo#22008 This ensures that Metro always and consistently emits "JSC-safe" (i.e., `//&` query-delimited) URLs where they are used as a "source URL" for evaluated JS. This includes: - `sourceURL` within the JSON HMR payload (`HmrModule`) - `//# sourceURL` comments within the body of a base or HMR bundle - The new `Content-Location` header delivered in response to an HTTP bundle request. Clients will be expected to use these as source URL arguments to JS engines, in preference to the URL on which they might have connected/requested the bundle originally. ``` * **[Fix]**: Emit source URLs in a format that will not be stripped by JavaScriptCore ``` Differential Revision: D45983876 fbshipit-source-id: cbf749bae0256bc981588fa2b9e7b0d60eba7461

…cebook#37501) Summary: Pull Request resolved: facebook#37501 This is the iOS side of the fix for facebook#36794. That issue aside for the moment, the high-level idea here is to conceptually separate the bundle *request URL*, which represents a request for the *latest* bundle, from the *source URL* passed to JS engines, which should represent the code actually being executed. In future, we'd like to use this to refer to a point-in-time snapshot of the bundle, so that stack traces more often refer to the code that was actually run, even if it's since been updated on disk (actually implementing this isn't planned at the moment, but it helps describe the distinction). Short term, this separation gives us a way to address the issue with JSC on iOS 16.4 by allowing Metro to provide the client with a [JSC-safe URL](react-native-community/discussions-and-proposals#646) to pass to the JS engine, even where the request URL isn't JSC-safe. We'll deliver that URL to the client on HTTP bundle requests via the [`Content-Location`](https://www.rfc-editor.org/rfc/rfc9110#name-content-location) header, which is a published standard for communicating a location for the content provided in a successful response (typically used to provide a direct URL to an asset after content negotiation, but I think it fits here too). For the long-term goal we should follow up with the same functionality on Android and out-of-tree platforms, but it's non-essential for anything other than iOS 16.4 at the moment. For the issue fix to work end-to-end we'll also need to update Metro, but the two pieces are decoupled and non-breaking so it doesn't matter which lands first. Changelog: [iOS][Changed] Prefer `Content-Location` header in bundle response as JS source URL Reviewed By: huntie Differential Revision: D45950661 fbshipit-source-id: a67fefadfda2d79009e1ce576e394661c1466601

…cebook#37501) Summary: Pull Request resolved: facebook#37501 This is the iOS side of the fix for facebook#36794. That issue aside for the moment, the high-level idea here is to conceptually separate the bundle *request URL*, which represents a request for the *latest* bundle, from the *source URL* passed to JS engines, which should represent the code actually being executed. In future, we'd like to use this to refer to a point-in-time snapshot of the bundle, so that stack traces more often refer to the code that was actually run, even if it's since been updated on disk (actually implementing this isn't planned at the moment, but it helps describe the distinction). Short term, this separation gives us a way to address the issue with JSC on iOS 16.4 by allowing Metro to provide the client with a [JSC-safe URL](react-native-community/discussions-and-proposals#646) to pass to the JS engine, even where the request URL isn't JSC-safe. We'll deliver that URL to the client on HTTP bundle requests via the [`Content-Location`](https://www.rfc-editor.org/rfc/rfc9110#name-content-location) header, which is a published standard for communicating a location for the content provided in a successful response (typically used to provide a direct URL to an asset after content negotiation, but I think it fits here too). For the long-term goal we should follow up with the same functionality on Android and out-of-tree platforms, but it's non-essential for anything other than iOS 16.4 at the moment. For the issue fix to work end-to-end we'll also need to update Metro, but the two pieces are decoupled and non-breaking so it doesn't matter which lands first. Changelog: [iOS][Changed] Prefer `Content-Location` header in bundle response as JS source URL Reviewed By: huntie Differential Revision: D45950661 fbshipit-source-id: 35682369b77303674028291c86ac386f4bd12ce8

…cebook#37501) Summary: Pull Request resolved: facebook#37501 This is the iOS side of the fix for facebook#36794. That issue aside for the moment, the high-level idea here is to conceptually separate the bundle *request URL*, which represents a request for the *latest* bundle, from the *source URL* passed to JS engines, which should represent the code actually being executed. In future, we'd like to use this to refer to a point-in-time snapshot of the bundle, so that stack traces more often refer to the code that was actually run, even if it's since been updated on disk (actually implementing this isn't planned at the moment, but it helps describe the distinction). Short term, this separation gives us a way to address the issue with JSC on iOS 16.4 by allowing Metro to provide the client with a [JSC-safe URL](react-native-community/discussions-and-proposals#646) to pass to the JS engine, even where the request URL isn't JSC-safe. We'll deliver that URL to the client on HTTP bundle requests via the [`Content-Location`](https://www.rfc-editor.org/rfc/rfc9110#name-content-location) header, which is a published standard for communicating a location for the content provided in a successful response (typically used to provide a direct URL to an asset after content negotiation, but I think it fits here too). For the long-term goal we should follow up with the same functionality on Android and out-of-tree platforms, but it's non-essential for anything other than iOS 16.4 at the moment. For the issue fix to work end-to-end we'll also need to update Metro, but the two pieces are decoupled and non-breaking so it doesn't matter which lands first. Changelog: [iOS][Changed] Prefer `Content-Location` header in bundle response as JS source URL Reviewed By: huntie Differential Revision: D45950661 fbshipit-source-id: 35912ab0d4669709619b8b55d00a1b3491344ed3

…7501) Summary: Pull Request resolved: #37501 This is the iOS side of the fix for #36794. That issue aside for the moment, the high-level idea here is to conceptually separate the bundle *request URL*, which represents a request for the *latest* bundle, from the *source URL* passed to JS engines, which should represent the code actually being executed. In future, we'd like to use this to refer to a point-in-time snapshot of the bundle, so that stack traces more often refer to the code that was actually run, even if it's since been updated on disk (actually implementing this isn't planned at the moment, but it helps describe the distinction). Short term, this separation gives us a way to address the issue with JSC on iOS 16.4 by allowing Metro to provide the client with a [JSC-safe URL](react-native-community/discussions-and-proposals#646) to pass to the JS engine, even where the request URL isn't JSC-safe. We'll deliver that URL to the client on HTTP bundle requests via the [`Content-Location`](https://www.rfc-editor.org/rfc/rfc9110#name-content-location) header, which is a published standard for communicating a location for the content provided in a successful response (typically used to provide a direct URL to an asset after content negotiation, but I think it fits here too). For the long-term goal we should follow up with the same functionality on Android and out-of-tree platforms, but it's non-essential for anything other than iOS 16.4 at the moment. For the issue fix to work end-to-end we'll also need to update Metro, but the two pieces are decoupled and non-breaking so it doesn't matter which lands first. Changelog: [iOS][Changed] Prefer `Content-Location` header in bundle response as JS source URL Reviewed By: huntie Differential Revision: D45950661 fbshipit-source-id: 170fcd63a098f81bdcba55ebde0cf3569dceb88d

…ook#989) Summary: Pull Request resolved: facebook#989 The remaining Metro part of the implementation of react-native-community/discussions-and-proposals#646, to fix (along with an RN change): - facebook/react-native#36794 and - expo/expo#22008 This ensures that Metro always and consistently emits "JSC-safe" (i.e., `//&` query-delimited) URLs where they are used as a "source URL" for evaluated JS. This includes: - `sourceURL` within the JSON HMR payload (`HmrModule`) - `//# sourceURL` comments within the body of a base or HMR bundle - The new `Content-Location` header delivered in response to an HTTP bundle request. Clients will be expected to use these as source URL arguments to JS engines, in preference to the URL on which they might have connected/requested the bundle originally. ``` * **[Fix]**: Emit source URLs in a format that will not be stripped by JavaScriptCore ``` Reviewed By: GijsWeterings Differential Revision: D45983876 fbshipit-source-id: b13c712521f7a3b6716c14d392cb3a057ede5936

… place of `?`) Summary: The first part of implementing react-native-community/discussions-and-proposals#646 to address facebook/react-native#36794. This allows Metro to respond to bundle and symbolication requests that use URLs with `//&` in place of `?` as a query delimiter. ``` **[Feature]**: Support URLs for both bundling and symbolication requests using `//&` instead of `?` as a query string delimiter ``` (Note: This does *not* add support for registering HMR entry points in the JSC-safe format - that's not necessary at this point, if at all, and I'm keen to minimise the footprint of this stack for easier backporting.) Reviewed By: huntie Differential Revision: D45983877 fbshipit-source-id: e799f76cd26c2ca8026b4d1bf70a582814ae1790

Summary: Pull Request resolved: #989 The remaining Metro part of the implementation of react-native-community/discussions-and-proposals#646, to fix (along with an RN change): - facebook/react-native#36794 and - expo/expo#22008 This ensures that Metro always and consistently emits "JSC-safe" (i.e., `//&` query-delimited) URLs where they are used as a "source URL" for evaluated JS. This includes: - `sourceURL` within the JSON HMR payload (`HmrModule`) - `//# sourceURL` comments within the body of a base or HMR bundle - The new `Content-Location` header delivered in response to an HTTP bundle request. Clients will be expected to use these as source URL arguments to JS engines, in preference to the URL on which they might have connected/requested the bundle originally. ``` * **[Fix]**: Emit source URLs in a format that will not be stripped by JavaScriptCore ``` Reviewed By: GijsWeterings Differential Revision: D45983876 fbshipit-source-id: 3e7f0118091424b9c1b1d40e4eb7baeb5be1f48f